-
Notifications
You must be signed in to change notification settings - Fork 38
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Reduced props accepted by ColumnHeader
#2287
base: main
Are you sure you want to change the base?
Conversation
…umnheader-props
…umnheader-props
areFiltersSet, | ||
hasAnySubRows, | ||
headers, | ||
state, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can state
also be removed? I assume we can get it from instance
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would you mind elaborating more on getting state
from instance
? Just want to make sure I get it right.
(Edited)
So I currently pass in instance
and removed both state
and visibleColumns
since these can be derived from instance
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since instance
is a Table-wide concept, I think we can store it in context.
I see there is a TableColumnsContext
that stores instance.columns
. We can rename this to TableInstanceContext
and store instance
instead.
One thing that we'll need to verify is whether instance
is a stable/memoized object, i.e. it doesn't change on every re-render.
@@ -47,16 +45,15 @@ export const ColumnHeader = < | |||
const { | |||
columnRefs, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can columnRefs
be removed? It doesn't feel quite right for a ColumnHeader
to mutate its parent component's variables.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have removed this prop.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You have removed the whole functionality, not just the prop. What I was asking for is to move the columnRefs
-related code from ColumnHeader
into Table
.
You can pass a ref
from the outside.
ref={(el) => {
if (el) {
columnRefs.current[column.id] = el;
}
}}
And the refs will need to be merged inside ColumnHeader
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, no more comments atm. Might have some after you address the existing open comments.
…winUI into uyen/columnheader-props
Changes
Reduced the number of props accepted by
ColumnHeader
as some of these props are not specific to individual columns. (mentioned here #2178 (review))Testing
N/A
Docs
N/A